Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

implement symmetric encryption #3998

Merged
merged 8 commits into from
May 10, 2016
Merged

implement symmetric encryption #3998

merged 8 commits into from
May 10, 2016

Conversation

chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented Apr 30, 2016

@rohitpaulk
Copy link
Contributor

Rebased on master, previous head was @4a1097b8a007bb2e7256aa3d57a9539e182454a9

@chadwhitacre
Copy link
Contributor Author

Alright! This one's up—and it's the big one!

@rohitpaulk @aandis et al. What do we think of our encryption procedure here?

@chadwhitacre
Copy link
Contributor Author

cc: @gratipay/security @TheHmadQureshi

@chadwhitacre chadwhitacre force-pushed the symmetric-encryption branch from 08b3490 to 01b4ec3 Compare May 1, 2016 23:00
@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 1, 2016

Rebased again (post deploy of #4004), previous head was 08b3490.


def test_ep_packs_encryptingly(self):
packed = Participant.encrypting_packer.pack({"foo": "bar"})
assert urlsafe_b64decode(packed)[0] == b'\x80' # Frenet version
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frenet → Fernet

@chadwhitacre
Copy link
Contributor Author

Do we trust Fernet? Do we trust cryptography?

@chadwhitacre chadwhitacre modified the milestone: Bring Back Payroll for Team Gratipay May 4, 2016
@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 4, 2016

Bottom of https://cryptography.io/en/latest/:

cryptography has not been subjected to an external audit of its code or documentation. If you’re interested in discussing an audit please get in touch.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 4, 2016

Investigations

Fernet
cryptography
Etc.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 4, 2016

I noticed a security-hardening label on cryptography with three tickets. Two are not blockers for us (one, two), and the third is also covered by a search for fernet.

screen shot 2016-05-04 at 12 07 40 pm

@chadwhitacre
Copy link
Contributor Author

Basically we need to decide if we're going to trust a) Fernet, and b) cryptography's implementation of Fernet.

@chadwhitacre
Copy link
Contributor Author

cryptography has a mailing list in addition to GitHub issues. We'll need to dig there, too, probably especially for the "why" about fernet.

@chadwhitacre
Copy link
Contributor Author

Fernet looks like a rinky-dink operation. It has 44 stars and 7 forks and approximately one contributor.

screen shot 2016-05-04 at 12 22 46 pm

@chadwhitacre
Copy link
Contributor Author

The author doesn't mention fernet on his blog and barely on twitter. In fact:

Then I’ll continue to think fernet is a bad format shrug

https://twitter.com/dstufft/status/437363105228939264

Hmmm ...

@chadwhitacre
Copy link
Contributor Author

Other end of that Twitter convo suggests that Fernet comes from Heroku (where kr works):

Nice callout to Fernet by @/alex_gaynor in his #PyTN2014 keynote which was adapted from @/heroku’s Ruby Fernet

https://twitter.com/craigkerstiens/status/437352536303874048

P.S. Should I/we be @mention'ing folks here? Would that be spam or transparency?

@chadwhitacre
Copy link
Contributor Author

I mean, PyCA advertises their interest in third-party audits.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 4, 2016

Searching fernet is not so helpful ;-) , but heroku fernet turns up some hits:

@chadwhitacre
Copy link
Contributor Author

"I don't have any issues with the code [on this PR], it seems simple enough. The only problem is this Fernet—how secure and reliable it is." —@kaguillera

@chadwhitacre
Copy link
Contributor Author

I've started a doc on credential management: gratipay/inside.gratipay.com#606.

@chadwhitacre
Copy link
Contributor Author

In a secure system, Alice and Mallory’s records probably wouldn’t be encrypted using the same key. [p 57]

#3998 (comment)

Transaction level key: In this practice, each transaction is encrypted with its own unique key, leading to higher level of data breach mitigation.

#3998 (comment)

I don't see how we would do this apart from a key management application such as StrongAuth or Porticor, and I think that's something to reticket and tackle further down the line.

@chadwhitacre
Copy link
Contributor Author

@chadwhitacre
Copy link
Contributor Author

All tax forms are well protected because they are encrypted with RSA 2048-bit encryption.

https://www.reddit.com/r/patreon/comments/3tofi7/patreon_hacked_now_someone_is_threatening_to_leak/cx84dir

Hmmm ... I was wondering about that: should we be using asymmetric encryption? Web app encrypts, other processes decrypt?

@chadwhitacre
Copy link
Contributor Author

I did some spelunking in the Patreon data dump. They do appear to have used RSA. While their implementation does have API for both encrypting and decrypting, I don't see that they do actually decrypt anywhere in the web application. This makes it possible for them to distribute a public key with the application, and keep the private key completely separate.

This works for their model, where all creators have a direct relationship with Patreon. On Gratipay we're facilitating a relationship between a ~user and a Team, which is an organization other than Gratipay. We need to be able to communicate full identity information from the ~user to the Team (e.g., W-9 in the U.S.), and vice versa. In order to do that, we need to be able to decrypt within the application, so even if we used asymmetric encryption, we wouldn't be able to keep the private key entirely away from the web app.

This is the one I actually reviewed.
@chadwhitacre
Copy link
Contributor Author

Okay! The investigations and punchlist items are all checked off. I'm pretty sure that means that this is ready for final review and merge. 😳

Who is going to be brave enough? @rohitpaulk? @aandis?

@chadwhitacre
Copy link
Contributor Author

There's a bottle of fernet here at Stack, waiting to celebrate ...

fernet


"""

def __init__(self, key, *old_keys):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be simplified to just take *keys?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe this was done to emphasize the answer to https://github.com/gratipay/gratipay.com/pull/3998/files#r62548400?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's more or less what I was thinking. I wanted to differentiate between the first key, which is the one used for encryption (and decryption), and the old keys, which can only be used for decryption.

@rohitpaulk
Copy link
Contributor

This looks fine to me overall - I've left a few comments regarding how multiple keys will be handled, would be great if we could have more tests for that

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented May 9, 2016

Awesome, thanks! I've updated the punchlist in the description, and will get another commit or two together here ...

@chadwhitacre
Copy link
Contributor Author

@rohitpaulk Punchlist updated with responses, I'll let you check them off if you approve. :)

@rohitpaulk rohitpaulk merged commit 1bb41d3 into master May 10, 2016
@aandis
Copy link
Contributor

aandis commented May 10, 2016

!m @whit537 @rohitpaulk

@chadwhitacre
Copy link
Contributor Author

Ooooooh wow. Oh wowohwowohwow. 😳

Here we go!

!m @rohitpaulk @kaguillera @aandis *

clone1018 added a commit that referenced this pull request May 14, 2016
Update dependency docs in light of #3998
@dannluciano
Copy link

@whit537 thanks for this nice search report about symmetric encryption.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants